Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update toolchain #847

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Update toolchain #847

wants to merge 1 commit into from

Conversation

andrewmilson
Copy link
Contributor

@andrewmilson andrewmilson commented Sep 22, 2024

This change is Reviewable

@andrewmilson andrewmilson requested a review from alonh5 September 22, 2024 10:04
@andrewmilson andrewmilson force-pushed the andrew/dev/update-toolchain branch 4 times, most recently from ca1692e to 0fae216 Compare September 22, 2024 21:46
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 8 lines in your changes missing coverage. Please review.

Project coverage is 92.85%. Comparing base (8c8b744) to head (1f91c92).
Report is 19 commits behind head on dev.

Files with missing lines Patch % Lines
crates/prover/src/core/backend/simd/utils.rs 66.66% 7 Missing ⚠️
crates/prover/src/core/pcs/verifier.rs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #847      +/-   ##
==========================================
+ Coverage   91.90%   92.85%   +0.94%     
==========================================
  Files          92       91       -1     
  Lines       12604    12166     -438     
  Branches    12604    12166     -438     
==========================================
- Hits        11584    11297     -287     
+ Misses        910      797     -113     
+ Partials      110       72      -38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@andrewmilson andrewmilson force-pushed the andrew/dev/update-toolchain branch from 0fae216 to 6c25d63 Compare September 22, 2024 21:50
Copy link
Contributor

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 38 files at r1, 2 of 7 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 8 of 44 files reviewed, 2 unresolved discussions (waiting on @andrewmilson)


crates/prover/Cargo.toml line 57 at r3 (raw file):

rust-2018-idioms = "deny"
unused = "deny"

Rebase and remove:

[package.metadata.cargo-machete]
ignored = ["downcast-rs"]

crates/prover/src/core/backend/cpu/quotients.rs line 80 at r3 (raw file):

/// Precomputes the complex conjugate line coefficients for each column in each sample batch.
///
/// For the `i`-th (in a sample batch) column's numerator term `alpha^i * (c * F(p) - (a * p.y +

Why am I seeing changes in comments and other changes in the code? Is it somehow related to the toolchain update?

@andrewmilson andrewmilson force-pushed the andrew/dev/update-toolchain branch from 6c25d63 to 101a6c4 Compare September 25, 2024 08:27
Copy link
Contributor Author

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 44 files reviewed, 2 unresolved discussions (waiting on @alonh5)


crates/prover/Cargo.toml line 57 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Rebase and remove:

[package.metadata.cargo-machete]
ignored = ["downcast-rs"]

Done.


crates/prover/src/core/backend/cpu/quotients.rs line 80 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why am I seeing changes in comments and other changes in the code? Is it somehow related to the toolchain update?

The new toolchain was throwing errors for comments.
Mainly complaining the summaries were too long and needing a empty line in places.

Copy link
Contributor

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 16 of 38 files at r1, 4 of 7 files at r2, 18 of 18 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @andrewmilson)


.github/workflows/ci.yaml line 163 at r4 (raw file):

    steps:
      - uses: actions/checkout@v4
<<<<<<< HEAD

Resolve.

Code quote:

<<<<<<< HEAD

crates/prover/src/lib.rs line 1 at r4 (raw file):

#![allow(incomplete_features)]

Can you pleae explain the changes here?


crates/prover/src/core/backend/simd/m31.rs line 288 at r4 (raw file):

}

cfg_if::cfg_if! {

Is this also something that the new toolchain required?


crates/prover/src/core/backend/simd/lookups/gkr.rs line 28 at r4 (raw file):

        // Start DP with CPU backend to avoid dealing with instances smaller than a SIMD vector.
        let (y_rem, y_last_chunk) = y.split_last_chunk::<{ LOG_N_LANES as usize }>().unwrap();

How did you catch this? Did tests fail?


crates/prover/src/core/pcs/mod.rs line 39 at r4 (raw file):

            pow_bits: 5,
            // fri_config: FriConfig::new(0, 1, 3),
            fri_config: FriConfig::new(0, 1, 50),

On purpose?

Code quote:

            // fri_config: FriConfig::new(0, 1, 3),
            fri_config: FriConfig::new(0, 1, 50),

crates/prover/src/core/vcs/blake2s_ref.rs line 34 at r4 (raw file):

#[inline(always)]
fn rot16(x: u32) -> u32 {
    x.rotate_left(16)

Why is this left and all the others right? is it because for 16 it's identical?

@andrewmilson andrewmilson force-pushed the andrew/dev/update-toolchain branch from 101a6c4 to 52d050c Compare September 25, 2024 09:55
Copy link
Contributor Author

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 41 of 44 files reviewed, 6 unresolved discussions (waiting on @alonh5)


.github/workflows/ci.yaml line 163 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Resolve.

Done.


crates/prover/src/lib.rs line 1 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Can you pleae explain the changes here?

stdarch_x86_avx512 replaces stdsimd as the feature to use the avx512 intrinsics.
Added as cfg_attr since stdarch_x86_avx512 feature only exists on x86


crates/prover/src/core/backend/simd/m31.rs line 288 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Is this also something that the new toolchain required?

Not entirely but made sense to combine the change.
The error the toolchain had was about Swizzle, InterleaveEvens, InterleaveOdds being unused on non avx2/avx512


crates/prover/src/core/backend/simd/lookups/gkr.rs line 28 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

How did you catch this? Did tests fail?

Yes exactly.
No compile issue so was a bit perplexed at the test failures initially.


crates/prover/src/core/pcs/mod.rs line 39 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

On purpose?

My bad. Thanks


crates/prover/src/core/vcs/blake2s_ref.rs line 34 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why is this left and all the others right? is it because for 16 it's identical?

Oh funny, didn't notice this. Yeah I guess they are.

Rust analyser detected the bit shifts as a rotate and offered a quick fix which I applied.
Just changed it to rotate_right to be consistent.

Copy link
Contributor

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @andrewmilson)


crates/prover/src/core/backend/simd/lookups/gkr.rs line 28 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Yes exactly.
No compile issue so was a bit perplexed at the test failures initially.

Weird they would change the order like that.


crates/prover/src/core/vcs/blake2s_ref.rs line 34 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Oh funny, didn't notice this. Yeah I guess they are.

Rust analyser detected the bit shifts as a rotate and offered a quick fix which I applied.
Just changed it to rotate_right to be consistent.

Great thanks :)

Copy link
Contributor

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this PR be closed?

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @andrewmilson)

@alonh5 alonh5 closed this Oct 29, 2024
@ShuangWu121
Copy link

Hi, would the toolchain be updated later though? it is a bit hard to integrate stwo to other projects due to the toolchain "nightly-2024-01-04", would be nice to have a newer version. :)

@andrewmilson andrewmilson reopened this Nov 6, 2024
@andrewmilson andrewmilson force-pushed the andrew/dev/update-toolchain branch 7 times, most recently from 97084f0 to 44a5c93 Compare November 7, 2024 14:14
@shaharsamocha7 shaharsamocha7 force-pushed the andrew/dev/update-toolchain branch from 44a5c93 to e6d10bc Compare November 10, 2024 07:44
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: e6d10bc Previous: f6214d1 Ratio
SecureField add 103999700 ns/iter (± 1387260) 22737535 ns/iter (± 211082) 4.57

This comment was automatically generated by workflow using github-action-benchmark.

CC: @shaharsamocha7

@alonh5 alonh5 requested review from alonh5 and removed request for alonh5 November 12, 2024 10:51
@ShuangWu121
Copy link

ShuangWu121 commented Nov 17, 2024

Hi! Nice this PR is opened again! Great job!

Our repo now is using this branch (andrew/dev/update-toolchain) due to the old tool-chain of dev branch is not compatible with our project, so we really look forward for this PR to be merged.
But if it is not happening soon, would be possible to keep this branch updated with the dev branch? I see recently there are new features of preprocessed columns are added on dev, we'd really wish to use this new feature from this branch ^^
Thanks in advance!

@alon-dotan-starkware alon-dotan-starkware force-pushed the andrew/dev/update-toolchain branch from 3baf7ae to 1f91c92 Compare November 24, 2024 14:48
github-merge-queue bot pushed a commit to powdr-labs/powdr that referenced this pull request Dec 11, 2024
# Add Support for Constant Columns in Stwo Backend

Stwo's recent development introduced new APIs to support
constant/pre-processed columns. However, the `dev` branch of Stwo is
still using an older nightly toolchain, which is incompatible with
Powdr.

Currently, Stwo has two open PRs
[PR1](starkware-libs/stwo#847),
[PR2](starkware-libs/stwo#871) aimed at updating
the toolchain to nightly `11-06`. Previously, our Stwo backend
dependency relied on one of these PRs' branches. However, this branch is
no longer updated with the latest commits from their `dev` branch. These
new commits are required to support constant columns.

### Temporary Solution
To address this issue temporarily:
- I moved Stwo's dependency to my fork of Stwo, where the branch is
updated with both the latest `dev` branch commits and the newer
toolchain.

---

### Tasks in this PR
1. **Add APIs to support constant columns**:
2. **Update test cases**:
   - Modify and add test cases to validate constant column support.

---

---------

Co-authored-by: Thibaut Schaeffer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants